-
-
Notifications
You must be signed in to change notification settings - Fork 88
Feat/new component #685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feat/new component #685
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new ActionMenu component intended to replace Android’s native menu lists with Papillon-styled menus, and updates the Grades screens to use it instead of MenuView.
Changes:
- Added
ui/components/ActionMenu.tsxwith an iOSMenuViewpassthrough and a custom Android modal menu implementation. - Replaced
MenuViewusage withActionMenuin Grades header period selector and the Averages method selector.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| ui/components/ActionMenu.tsx | New cross-platform menu component; custom Android rendering and iOS passthrough. |
| app/(tabs)/grades/index.tsx | Swaps period selector from MenuView to ActionMenu. |
| app/(tabs)/grades/atoms/Averages.tsx | Swaps averages method selector from MenuView to ActionMenu and adds a missing action id. |
ui/components/ActionMenu.tsx
Outdated
| const primaryColor = colors.primary; | ||
| const cardColor = colors.card; | ||
|
|
||
| const triggerRef = useRef<View>(null); |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript strict mode is enabled (tsconfig.json sets strict: true), so useRef<View>(null) will be a type error because null isn’t assignable to View. Use a nullable ref type (e.g., useRef<View | null>(null)), and update the optional chaining accordingly if needed.
| const triggerRef = useRef<View>(null); | |
| const triggerRef = useRef<View | null>(null); |
ui/components/ActionMenu.tsx
Outdated
| return { | ||
| position: "absolute" as const, | ||
| top: position.y + position.height + 8, | ||
| left: Math.min(position.x, 100), | ||
| }; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left: Math.min(position.x, 100) clamps the menu’s left position to at most 100px, so triggers placed to the right will always render the menu at x=100. This looks unintended and will misplace the menu on many layouts. Clamp left within the screen bounds instead (e.g., using Dimensions.get('window').width and the menu width/padding), or align based on the trigger center.
| function open() { | ||
| setVisible(true); | ||
| setTimeout(() => { | ||
| triggerRef.current?.measureInWindow((x, y, width, height) => { | ||
| setPosition({ x, y, width, height }); |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
measureInWindow can be unreliable on Android if the referenced wrapper <View> is flattened out of the native hierarchy (RN collapsable optimization). Since the ref is attached to a wrapper with no styles, consider adding collapsable={false} (or a non-empty style) so measuring consistently returns correct coordinates.
ui/components/ActionMenu.tsx
Outdated
| } catch {} | ||
|
|
||
| interface MenuAction { | ||
| id: string; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id is required here, but existing MenuView usage in this repo sometimes omits id for grouping actions (e.g. actions that only contain subactions). If this component is meant to be a drop-in replacement, consider making id optional for non-selectable/group items and generating a stable key for rendering on Android.
| id: string; | |
| id?: string; |
| <TouchableOpacity | ||
| onPress={() => setSubmenu(null)} | ||
| style={styles.header} | ||
| > |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submenu navigation only supports a single level: the “back” handler always does setSubmenu(null), so nested submenus can’t return to their parent submenu. If multi-level subactions are possible, track a stack of menus (or parent references) so back returns to the previous submenu.
ui/components/ActionMenu.tsx
Outdated
| return { | ||
| position: "absolute" as const, | ||
| top: position.y + position.height + 8, | ||
| left: Math.min(position.x, 100), | ||
| }; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Menu positioning only places the menu below the trigger (top: y + height + 8) without any screen-bound clamping. If the trigger is near the bottom of the screen, the menu will render off-screen. Consider clamping top (and potentially flipping above the trigger when there isn’t enough space) based on window height and measured menu height.
ui/components/ActionMenu.tsx
Outdated
| image?: string; | ||
| imageColor?: string; | ||
| destructive?: boolean; | ||
| disabled?: boolean; | ||
| subactions?: MenuAction[]; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other MenuView call sites in this repo use attributes: { destructive, disabled }. The Android renderer here only reads action.destructive / action.disabled, so those existing actions won’t behave the same if migrated. Consider supporting the library’s attributes field and mapping it to these flags.
Dev-LeChacal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magnifique !
LGTM
|
#581 |
Dev-LeChacal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utilise l'ActionMenu dans le component ChipButton qui utilise aussi le MenuView
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| function open() { | ||
| setVisible(true); | ||
| setTimeout(() => { | ||
| triggerRef.current?.measureInWindow((x, y, width, height) => { | ||
| setPosition({ x, y, width, height }); | ||
| }); | ||
| }, 0); | ||
| } | ||
|
|
||
| function close() { | ||
| setVisible(false); | ||
| setSubmenuStack([]); | ||
| } |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open() schedules a setTimeout that calls setPosition but it’s never cleared on unmount, and position is not reset on close(). This can cause setState-after-unmount warnings and stale positioning on the next open. Consider using requestAnimationFrame (or storing/clearing the timeout id in an effect cleanup) and resetting position to null when closing/opening.
ui/components/ActionMenu.tsx
Outdated
| interface MenuAction { | ||
| id?: string; | ||
| title: string; | ||
| subtitle?: string; | ||
| state?: "on" | "off" | "mixed"; | ||
| image?: string; | ||
| imageColor?: number | ColorValue; | ||
| destructive?: boolean; | ||
| disabled?: boolean; | ||
| subactions?: MenuAction[]; | ||
| displayInline?: boolean; | ||
| attributes?: { | ||
| destructive?: boolean; | ||
| disabled?: boolean; | ||
| state?: "on" | "off" | "mixed"; | ||
| }; | ||
| } | ||
|
|
||
| interface ActionMenuProps { | ||
| actions: MenuAction[]; | ||
| children: ReactNode; | ||
| onPressAction?: (event: { nativeEvent: { event: string } }) => void; | ||
| title?: string; | ||
| } |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MenuAction is redefined locally but other call sites (e.g. ChipButton) still use MenuAction from @react-native-menu/menu. This duplication can drift and causes avoidable type incompatibilities. Consider exporting a single MenuAction type from ActionMenu (or re-exporting the library type) and using it consistently for actions across the app.
| @@ -1,5 +1,5 @@ | |||
| import { Papicons } from "@getpapillon/papicons"; | |||
| import { MenuAction, MenuView } from '@react-native-menu/menu'; | |||
| import { MenuAction } from '@react-native-menu/menu'; | |||
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MenuAction is only used as a TypeScript type here. Use a type-only import (import type) to ensure it’s erased at runtime and to match existing project patterns (e.g. ui/components/AlertProvider.tsx).
| import { MenuAction } from '@react-native-menu/menu'; | |
| import type { MenuAction } from '@react-native-menu/menu'; |
| <ActionMenu | ||
| onPressAction={({ nativeEvent }) => { | ||
| const actionId = nativeEvent.event; |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description says MenuView usages are replaced by ActionMenu, but there are still multiple MenuView imports/usages in the repo (e.g. app/(features)/(news)/specific.tsx, app/(features)/attendance.tsx, app/(modals)/profile.tsx, app/(modals)/wallpaper.tsx, app/(tabs)/calendar/event/[id].tsx). Either update the description/scope, or complete the replacement in this PR/follow-up.
| <View | ||
| ref={triggerRef} | ||
| collapsable={false} | ||
| onTouchEnd={(e) => { | ||
| if (!visible) { | ||
| e.stopPropagation(); | ||
| open(); | ||
| } | ||
| }} |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Android, the menu is opened via the wrapper View’s onTouchEnd. This is likely to never fire when the child is a Touchable*/Pressable (e.g. TabHeaderTitle is a TouchableOpacity with a default onPress), which would prevent the menu from opening. Consider using responder capture (onStartShouldSetResponderCapture/onResponderRelease) or an explicit Pressable trigger wrapper so the ActionMenu reliably receives the gesture, and gate it behind actions.length > 0 to avoid intercepting taps when there’s no menu.
raphckrman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, merci pour ta Pull Request qui vise à résoudre un problème important sur Android, j'ai fait quelques petits commentaires pour améliorer tout ça, il faudrait aussi faire attention aux safe-areas, par exemple sur un appareil de développement, le menu passe dans l'encoche de la caméra.
ui/components/ActionMenu.tsx
Outdated
| let NativeMenuView: any = null; | ||
| try { | ||
| NativeMenuView = require("@react-native-menu/menu").MenuView; | ||
| } catch {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faudrait peut-être ajouter une gestion des erreurs ici, et peut-être de charger MenuView uniquement si l'appareil est sous iOS puisqu'il est utilisé seulement dans ce cas?
ui/components/ActionMenu.tsx
Outdated
| } from "react-native"; | ||
| import { useTheme } from "@react-navigation/native"; | ||
|
|
||
| let NativeMenuView: any = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faudrait retirer le any pour le remplacer par un typage plus strict
ui/components/ActionMenu.tsx
Outdated
| interface MenuAction { | ||
| id?: string; | ||
| title: string; | ||
| subtitle?: string; | ||
| state?: "on" | "off" | "mixed"; | ||
| image?: string; | ||
| imageColor?: number | ColorValue; | ||
| destructive?: boolean; | ||
| disabled?: boolean; | ||
| subactions?: MenuAction[]; | ||
| displayInline?: boolean; | ||
| attributes?: { | ||
| destructive?: boolean; | ||
| disabled?: boolean; | ||
| state?: "on" | "off" | "mixed"; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est super le fait que ce soit les mêmes props entre la version native et ta version, mais pourquoi ne pas réutiliser les types de MenuView? Ça pourrait éviter de dupliquer du code et ça assure une cohérence à 100% avec la librairie native dans le cas où l'utilisateur est sous iOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui c'est vrai que c'est plus malin merci!
ui/components/ActionMenu.tsx
Outdated
| interface ActionMenuProps { | ||
| actions: MenuAction[]; | ||
| children: ReactNode; | ||
| onPressAction?: (event: { nativeEvent: { event: string } }) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| onPressAction?: (event: { nativeEvent: { event: string } }) => void; | |
| onPressAction?: ({ nativeEvent }: NativeActionEvent) => void; |
ui/components/ActionMenu.tsx
Outdated
| interface ActionMenuProps { | ||
| actions: MenuAction[]; | ||
| children: ReactNode; | ||
| onPressAction?: (event: { nativeEvent: { event: string } }) => void; | ||
| title?: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce serait aussi cool ici d'utiliser les types de la librairie native
ui/components/ActionMenu.tsx
Outdated
| style={styles.header} | ||
| > | ||
| <Text style={[styles.back, { color: textColor }]}> | ||
| ‹ {currentSubmenu.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On retrouve le même problème avec ce symbole Unicode, une Papicons ne ferait pas mieux l'affaire?
ui/components/ActionMenu.tsx
Outdated
| minWidth: 260, | ||
| maxWidth: 320, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu es sûr de ces valeurs ? Papillon est utilisé sur beaucoup d'appareils différents, ça me semble un peu risqué codé ça en dur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oui j'y ai pensé mais je ne sais pas trop comment remplacer ça si tu a une idée
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ui/components/ActionMenu.tsx
Outdated
| paddingVertical: 10, | ||
| paddingHorizontal: 14, | ||
| borderBottomWidth: StyleSheet.hairlineWidth, | ||
| borderBottomColor: "rgba(128,128,128,0.2)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il me semble que Papillon a une couleur précise pour les bordures, il faudrait peut-être voir pour l'utiliser, ça permet de modifier partout et rester dans le theme de Papillon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oui j'ai trouvé dans un autre composant: borderBottomColor: (Platform.OS === 'ios' && !runsIOS26) ? colors.border : undefined,
ui/components/ActionMenu.tsx
Outdated
| flexDirection: "row", | ||
| alignItems: "center", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c'est typiquement ce que peut faire Stack, tu gagnerais en lisibilité etc en utilisant ce composant
| borderRadius: 10, | ||
| }, | ||
| itemSelected: { | ||
| backgroundColor: "rgba(0,102,204,0.12)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je suis pas méga fan de la couleur, et du fait qu'elle soit en dur dans le code, un avis sur ça @ecnivtwelve ou @tom-things ?
…enuView only on iOS
… and fixes the errors in this first commit.
…u in wallpaper.tsx
…Menu in attendance.tsx
…ctionMenu in [id].tsx
…ctionMenu in specific.tsx
raphckrman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, y'a eu pas mal de changements, c'est possible d'avoir un screen de comment ça rend avec les Papicons? Tu as géré aussi pour pas les safe-areas ?
ui/components/ActionMenu.tsx
Outdated
| NativeMenuView = mod?.MenuView ?? null; | ||
| } catch (err: unknown) { | ||
| console.warn("ActionMenu: impossible de charger @react-native-menu/menu MenuView:", err); | ||
| NativeMenuView = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu l'as déjà mis en null juste avant
ui/components/ActionMenu.tsx
Outdated
| const mod = require("@react-native-menu/menu"); | ||
| NativeMenuView = mod?.MenuView ?? null; | ||
| } catch (err: unknown) { | ||
| console.warn("ActionMenu: impossible de charger @react-native-menu/menu MenuView:", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a une méthode dans le logger pour ça
ui/components/ActionMenu.tsx
Outdated
| } | ||
| } | ||
|
|
||
| type MenuAction = NativeMenuAction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi ne pas utiliser directement le type NativeMenuAction ?
ui/components/ActionMenu.tsx
Outdated
| interface ActionMenuProps { | ||
| actions: MenuAction[]; | ||
| children: ReactNode; | ||
| onPressAction?: ({ nativeEvent }: NativeActionEvent) => void; | ||
| title?: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce serait possible, dans la continuité de ce qui est déjà fait juste avant, de reprendre les props de la librairie native? Pour être sur que c'est le même fonctionnement
ui/components/ActionMenu.tsx
Outdated
| const colorText = action.imageColor | ||
| ? action.imageColor | ||
| : destructive | ||
| ? destructiveColor | ||
| : textColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l'opération ternaire est un peu chaotique, y'en a deux imbriqué



Règles de contribution
Caution
Afin de garantir une application stable et pérenne dans le temps, nous t'invitons à vérifier que tu as bien respecté les règles de contribution. Sans cela, ta Pull Request ne pourra pas être examinée.
Résumé des changements
Cette PR remplace les anciennes listes natives d'Android par des listes dans le style de Papillon.
MenuViewpar desActionMenuLe composant a été conçu pour être un remplacement direct des balises sans changer la structure de l'élément:
Capture(s) d'écran